No actionable comments were generated in the recent review. 🎉

sequenceDiagram
    participant Client
    participant SQS_Server
    participant Partitioner
    participant Visibility_Store
    participant Data_Store

    rect rgba(100, 150, 200, 0.5)
    Note over Client,SQS_Server: Partitioned FIFO Send
    Client->>SQS_Server: SendMessage(MessageGroupId="group-A")
    SQS_Server->>Partitioner: partitionFor(meta, "group-A")
    Partitioner-->>SQS_Server: partition = 2
    SQS_Server->>Data_Store: Write(sqsMsgDataKeyDispatch(..., partition=2, ...))
    SQS_Server->>Visibility_Store: Write(sqsMsgVisKeyDispatch(..., partition=2, ...))
    SQS_Server->>SQS_Server: encodeReceiptHandleDispatch(..., partition=2) → v2 handle
    SQS_Server-->>Client: SendMessageResponse(ReceiptHandle=v2)
    end

    rect rgba(200, 100, 150, 0.5)
    Note over Client,Visibility_Store: Partitioned Fanout Receive
    Client->>SQS_Server: ReceiveMessage(MaxMessages=10)
    SQS_Server->>SQS_Server: readTS → startPartitionOffset → partition[0..N)
    loop For Each Partition
        SQS_Server->>Visibility_Store: Scan(sqsMsgVisScanBoundsDispatch(..., partition, ...))
        Visibility_Store-->>SQS_Server: Candidates{partition, visKey, messageID}
    end
    SQS_Server->>Data_Store: Load(sqsMsgDataKeyDispatch(..., cand.partition, ...))
    SQS_Server->>SQS_Server: encodeReceiptHandleDispatch(..., cand.partition) → v2 handle
    SQS_Server-->>Client: ReceiveMessageResponse(Messages=[...], ReceiptHandles=[v2])
    end

    rect rgba(150, 200, 100, 0.5)
    Note over Client,Data_Store: Partition-Aware Delete
    Client->>SQS_Server: DeleteMessage(ReceiptHandle=v2)
    SQS_Server->>SQS_Server: validateReceiptHandleVersion(meta, v2_handle)
    SQS_Server->>Data_Store: Delete(sqsMsgDataKeyDispatch(..., v2_handle.partition, ...))
    SQS_Server->>Visibility_Store: Delete(sqsMsgVisKeyDispatch(..., v2_handle.partition, ...))
    SQS_Server-->>Client: DeleteMessageResponse()
    end

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@claude review

Claude finished @bootjp's task in 8m 28s —— View job

Code Review: feat(sqs): wire partitioned-FIFO data plane through dispatch helpers (Phase 3.D PR 5b-2)

• Gather PR context and understand the request
• Read changed files (9 adapter files, ~900 additions / ~160 deletions)
• Analyze dispatch helpers and wiring logic
• Review test coverage
• Check for data loss, concurrency, and consistency issues
• Post complete review

Overall assessment

This is a well-structured, carefully staged PR. The core abstraction — a single dispatch layer (sqs_keys_dispatch.go) that routes to legacy or partitioned key constructors based on meta.PartitionCount — is the right shape, and the dormancy guarantee (no observable change on production queues) holds cleanly. The self-review in the PR description is thorough and accurate on all five CLAUDE.md lenses. There is one medium-severity finding and a few minor notes.

Medium: validateReceiptHandleVersion does not bound-check handle.Partition against meta.PartitionCount

File: adapter/sqs_keys_dispatch.go:165–196

validateReceiptHandleVersion verifies that the handle's version matches the queue's partition mode, but it does not verify that the partition index in a v2 handle is within [0, meta.PartitionCount).

A client that receives a legitimate v2 handle (partition = N) could forge a new v2 handle for the same queue with an out-of-range partition (e.g., partition = 63 on a 4-partition queue). After passing the version check, loadMessageForDelete constructs a key for partition 63, gets ErrKeyNotFound, and returns sqsDeleteNoOp — silently succeeding. This is AWS-idempotent but slightly incorrect semantics for a forged handle. More importantly, it leaks structural information: the client can confirm partition-key-space boundaries exist at that queue by observing the silent success vs. an explicit invalid-handle error.

// Suggested addition in validateReceiptHandleVersion, after the version checks:
if meta != nil && meta.PartitionCount > 1 && handle.Partition >= meta.PartitionCount {
    return errors.New("receipt handle partition out of range")
}

Since PartitionCount is immutable after queue creation (enforced by validatePartitionImmutability), this check is race-safe. The dormancy gate in PR 5b-2 means this can't be triggered today, but it should be in place before the gate is lifted in PR 5b-3.

Fix this →

Minor observations

1. now is computed per-partition in scanAndDeliverPartition (sqs_messages.go:901)

Before this PR, now was computed once in scanAndDeliverOnce. After, it is computed fresh at the start of each scanAndDeliverPartition call, meaning a message that crosses its visibility threshold between partitions 0 and 1 during a fanout receive might be delivered in partition 1's scan but would not have been found in a single-partition equivalent. This is strictly a behavior improvement (AWS does not guarantee anything stronger than eventual delivery anyway), but it is a subtle semantic delta worth noting in the PR description for future readers.

2. Integration test's legacy-keyspace probe end key uses 4 bytes of 0xFF (sqs_partitioned_dispatch_test.go:184)

end = append(end, 0xFF, 0xFF, 0xFF, 0xFF)

The actual message-ID suffix in the key is a base64url-encoded 16-byte UUID (~22 chars). The 4-byte 0xFF suffix works correctly because all base64url characters (A-Z, a-z, 0-9, -, _) have byte values < 0xFF, so any base64url string of any length compares less than [0xFF 0xFF 0xFF 0xFF]. However, using prefixScanEnd(legacyDataPrefix) (already available in the codebase at adapter/prefix_scan.go) would make the intent explicit and match the pattern used in the rest of the reaper code. Not a bug, just a consistency nit.

3. sqsMsgVisScanBoundsDispatch duplicates the scan-bounds logic from sqsMsgVisScanBounds (sqs_keys_dispatch.go:128–141)

The two functions share identical logic (construct prefix, zero-pad start, increment upper bound for end). A minor refactor opportunity: sqsMsgVisScanBoundsDispatch could delegate to sqsMsgVisScanBounds on the legacy branch rather than re-implementing the bounds math inline. The test TestSQSMsgVisScanBoundsDispatch_LegacyMatchesLegacy catches any divergence, so this is low risk either way.

Correctness verification (key paths)

Snapshot consistency: readTS is computed once in scanAndDeliverOnce and threaded identically through all partition fanout calls — consistent multi-partition snapshot. ✅

Group-lock partition coherence: partitionFor is deterministic (FNV-1a, power-of-two mask); a given MessageGroupId maps to exactly one partition for the lifetime of a queue generation. The group lock is always written and read at that same partition. ✅

PartitionCount immutability: validatePartitionImmutability is enforced on SetQueueAttributes, making the meta-dispatch pattern race-safe without re-loading meta inside OCC transactions. ✅

Dormancy preservation: Every code path that could emit a v2 handle is gated on meta.PartitionCount > 1, which CreateQueue continues to reject via validatePartitionDormancyGate until PR 5b-3. ✅

Test coverage

10 new tests are well-targeted. Particular highlights:

• TestSQSMsgVisScanBoundsDispatch_PerQueueOnPartitionedKeyspace directly pins the forward-note invariant from PR 731 round 2 (perQueue + partitioned keyspace must scan partitioned prefix, not legacy).
• TestSQSServer_PartitionedFIFO_SendReceiveDeleteRoundTrip sanity-checks the legacy keyspace is empty after all operations, which is the cleanest possible regression gate for future dispatch regressions.
• TestDecodeClientReceiptHandle_AcceptsV2 correctly documents the contract shift.

Missing: a test that a v2 handle with Partition >= meta.PartitionCount is rejected (tracking the medium finding above).

Verdict: Ready to merge after the partition bounds check is added to validateReceiptHandleVersion and covered by a test. The rest of the implementation is sound.

@claude review

Round 1 addressing Codex P1: rotate the receive-fanout starting partition by readTS so a sustained backlog on a single partition cannot starve higher-index partitions.

Changes:

• adapter/sqs_messages.go — new startPartitionOffset(partitions, readTS) helper. scanAndDeliverOnce now iterates (startOffset + i) % partitions. Implementation folds the upper and lower 32 bits of the HLC-derived readTS and mask-ANDs by partitions - 1 (power-of-two by validator invariant), keeping the computation in uint32 space (gosec G115 clean, no //nolint).
• adapter/sqs_partitioned_dispatch_test.go — TestStartPartitionOffset_RotatesByReadTS pins: legacy/perQueue → 0, every partition appears as a starting point across consecutive readTS, and the output is always in [0, partitions).

FIFO ordering: unaffected. partitionFor is deterministic, so a MessageGroupId always lands in one partition; cross-partition iteration order never reorders messages within any group.

Caller audit: scanAndDeliverOnce and scanAndDeliverPartition signatures and error semantics are unchanged — only the internal iteration order rotates. The new helper has one in-package caller.

P2/medium findings (out-of-range v2 partition validation, prefix-scan helper in test) are intentionally deferred — below the loop's P0/P1/high/critical/major threshold.

Claude finished @bootjp's task in 2m 22s —— View job

Code Review: Round 1 — Partition-starvation fix (startPartitionOffset)

• Gather PR context and understand the request
• Read adapter/sqs_messages.go — startPartitionOffset helper + fanout loop change
• Read adapter/sqs_partitioned_dispatch_test.go — TestStartPartitionOffset_RotatesByReadTS
• Review rotation logic correctness (gosec G115, power-of-two invariant)
• Review FIFO ordering preservation
• Review test coverage and edge cases
• Post complete review

Overall assessment

The fix is correct and well-scoped. The P1 finding (partition 0 starvation under sustained backlog) is addressed cleanly without touching any signatures or error semantics. Implementation and test match the stated intent. A few minor observations follow.

Correctness: startPartitionOffset

func startPartitionOffset(partitions uint32, readTS uint64) uint32 {
    if partitions <= 1 {
        return 0
    }
    var buf [8]byte
    binary.BigEndian.PutUint64(buf[:], readTS)
    folded := binary.BigEndian.Uint32(buf[0:4]) ^ binary.BigEndian.Uint32(buf[4:8])
    return folded & (partitions - 1)
}

Power-of-two invariant — PartitionCount is power-of-two enforced by htfifoValidatePartitionCount; effectivePartitionCount also returns 1 (= 2⁰) for the perQueue collapse, so mask-AND is equivalent to modulo throughout. ✅

gosec G115 avoidance — the byte-slice fold sidesteps the explicit uint64 → uint32 narrowing that gosec would flag. The approach correctly mirrors partitionFor. ✅

Distribution for real HLC values — the HLC layout is (physical_ms << 16) | logical_counter. Splitting at the 32-bit boundary gives:

• upper32 = physical_ms >> 16 (changes slowly, ~once per ms)
• lower32 = (physical_ms & 0xFFFF) << 16 | logical_counter (low 2 bits driven by logical_counter)

For consecutive receives in the same millisecond, logical_counter increments by 1 per call, so the low bits of folded increment by 1 → sequential cycling through all N partitions. ✅ For cross-millisecond boundaries, both halves change, maintaining spread.

Overflow safety in the loop — startOffset + i < 2 * partitions; with htfifoMaxPartitions = 32, max sum is 62, well within uint32. % partitions is safe since effectivePartitionCount guarantees partitions >= 1. ✅

Minor observations

1. Test exercises only low ts values where upper 32 bits are zero (sqs_partitioned_dispatch_test.go:335–353)

The coverage loop runs ts = 1..64 and ts = 0..1023. With these values, upper32 = 0, so folded = lower32 = ts, and the offset cycles trivially: ts & 3 = 0,1,2,3,0,.... This is correct but it doesn't exercise the XOR fold with non-zero upper bits. Real HLC timestamps have physical_ms >> 16 ≈ 0x190xxx (non-zero). Adding a sub-case with an HLC-range seed (e.g., ts = uint64(1_735_000_000_000) << 16) would give more confidence that the fold doesn't accidentally collapse to a constant offset for real values. The fix itself is still sound — this is a test quality observation only.

2. Comment verbosity (sqs_messages.go:907–925, sqs_messages.go:872–882)

CLAUDE.md's convention is: "one short line max" for comments. The startPartitionOffset doc block is ~10 lines and the inline loop comment is ~11 lines. The gosec-G115 reasoning is genuinely non-obvious and worth keeping, but both blocks could be compressed significantly. Not blocking, but the inline loop comment in particular restates what's already in the function's doc.

3. Loop uses % where mask-AND would be consistent (sqs_messages.go:892)

partition := (startOffset + i) % partitions

startPartitionOffset uses & (partitions - 1) and explicitly documents the power-of-two invariant. Using & (partitions - 1) in the loop too would be consistent and self-documenting, avoiding the % operator whose correctness requires knowing that partitions is never 0 (which it isn't, but the reader has to check). Either form is correct.

FIFO ordering

Preserved. partitionFor is deterministic (FNV-1a over MessageGroupId), so a group always hashes to exactly one partition regardless of which partition the fanout starts from. Re-ordering scanAndDeliverPartition calls cannot reorder messages within any group because cross-partition messages are by definition different groups. ✅

Summary

The P1 fix is correct, gosec-clean, and backward-compatible on every queue mode. The three observations above are all minor or nit-level. The implementation is ready to merge.

@claude review

Round 2 — addressed three CodeRabbit 🟠 Major findings:

1. validateReceiptHandleVersion now bounds-checks handle.Partition < meta.PartitionCount (the v2 partition is client-controlled once decoding accepts v2). Audit: both production callers (loadMessageForDelete, loadAndVerifyMessage) already surface any non-nil return as ReceiptHandleIsInvalid HTTP 400, so the new failure mode joins the existing public-error bucket — no caller-side semantic change.
2. Round-trip test now asserts parsed.Partition == partitionFor(meta, group) per message (was: only < 4), so a dispatch regression that collapsed every group to partition 0 would actually fail.
3. Post-delete emptiness check now probes the partitioned data keyspace directly per partition, and the public-API receive uses a 1s visibility timeout after a 1.1s sleep — a regression that left messages invisible but not deleted can no longer pass under the active 60s window.

Plus a new regression TestValidateReceiptHandleVersion_RejectsOutOfRangePartition covering several out-of-range partitions and the count-1 boundary.

Claude finished @bootjp's task in 3m 10s —— View job

Claude finished @bootjp's task ——

Code Review: Round 2 — Three Major Findings Addressed

• Gather PR context and understand Round 2 changes
• Read adapter/sqs_keys_dispatch.go — bounds check in validateReceiptHandleVersion
• Read adapter/sqs_keys_dispatch_v2_test.go — new regression test
• Read adapter/sqs_partitioned_dispatch_test.go — updated round-trip assertions + post-delete probe
• Verify correctness of partition bounds check
• Verify partitionFor(meta, group) assertion in round-trip test
• Verify post-delete emptiness check robustness
• Check for any remaining issues
• Post complete review

Overall assessment

All three 🟠 Major findings are correctly addressed. The implementations are sound, the new regression test is well-targeted, and no new issues were introduced. Two nits remain (carried from prior rounds but explicitly deferred). Ready to merge.

Finding 1: Bounds check in validateReceiptHandleVersion (sqs_keys_dispatch.go:194-196) ✅

The check is placed correctly — after the version check on the partitioned branch, before returning nil:

if handle.Partition >= meta.PartitionCount {
    return errors.New("receipt handle partition out of range for queue")
}

Placement is right: the bounds check only runs when version is already confirmed v2, so a v1 handle on a partitioned queue gets the version-mismatch error (not the partition error). Error string contains "out of range" as the test's strings.Contains expects. Both production callers (loadMessageForDelete → validateReceiptHandleVersion, loadAndVerifyMessage → validateReceiptHandleVersion) surface any non-nil error as ReceiptHandleIsInvalid, so the new failure mode joins the existing public error bucket without any caller-side change. ✅

TestValidateReceiptHandleVersion_RejectsOutOfRangePartition covers partitions 4, 5, 17, and 2³⁰ on a PartitionCount=4 queue (all must error) and the count-1 boundary partition=3 (must succeed). The boundary case is the right regression anchor. ✅

Finding 2: partitionFor(meta, group) dispatch assertion (sqs_partitioned_dispatch_test.go:154-159) ✅

The byMessageID reverse map and partitionFor assertion correctly close the gap the CodeRabbit review identified. The old parsed.Partition < 4 check would have passed even if sendFifoMessage always routed everything to partition 0 — this version fails that regression.

The meta loaded at line 96-101 (after installPartitionedMetaForTest completes) is the same meta the send path loads, since PartitionCount is immutable after creation. The assertion partitionFor(meta, group) == parsed.Partition is therefore a valid cross-check between send and receive dispatch. ✅

Finding 3: Post-delete emptiness check (sqs_partitioned_dispatch_test.go:180-210) ✅

The dual-probe strategy is solid:

• Direct keyspace scan per partition (lines 184-196): catches the "leave the record invisible but not deleted" regression that the previous immediate-receive check couldn't catch under a 60s visibility window. Uses sqsMsgDataKeyDispatch(meta, queueName, p, meta.Generation, "") — correctly routes to the partitioned key prefix for each partition, not the legacy prefix.
• Public-API receive with VisibilityTimeout: 1 after time.Sleep(1100 * time.Millisecond) (lines 198-210): catches any record that survived delete as invisible. The sleep margin is 100ms past the 1s timeout, which is adequate in a single-process test.

The scan iterates meta.PartitionCount (not effectivePartitionCount) — correct here since this is htfifoThroughputPerMessageGroupID where effectivePartitionCount == PartitionCount. ✅

Nits (not blocking, both carried from prior rounds)

1. New partitioned scan also uses 0xFF, 0xFF, 0xFF, 0xFF suffix (sqs_partitioned_dispatch_test.go:186-187)

The existing legacy scan (line 220-221) and the new per-partition scan both use:

end = append(end, 0xFF, 0xFF, 0xFF, 0xFF)

prefixScanEnd is available in the package (adapter/prefix_scan.go:7) and is already used throughout redis.go and dynamodb.go. The current approach is technically safe for this key format (message IDs are base64url, all bytes ≤ 0x7E; partition indices up to 32 never produce 0xFF bytes), but prefixScanEnd(dataPrefix) would make intent explicit and match the codebase pattern. Both occurrences would benefit from the same cleanup — a one-liner each.

2. Partition scan loop uses require.False(HasPrefix(...)) where require.Empty reads cleaner

The assertion at lines 190-195:

for _, kvp := range page {
    require.False(t,
        strings.HasPrefix(string(kvp.Key), string(dataPrefix)), ...)
}

Any record returned by ScanAt(dataPrefix, end, ...) necessarily has dataPrefix as a prefix, so HasPrefix is always true for any returned record — making require.False equivalent to t.Fatalf(...). The logic is correct (the test fails on the first surviving record with the key printed), but require.Empty(t, page, "partition %d still has records: %v", p, page) would be clearer. Minor style nit only.

Correctness table (Round 2 additions)

Verdict: Ready to merge.

@claude review

Round 3 — addressed two new Codex 🟠 P1 findings on the round-2 commit:

1. Dedup scoping (sqs_fifo.go:95): sqsPartitionedMsgDedupKey was (queue, partition, dedupID) — but DeduplicationScope=messageGroup (the only scope reachable on partitioned queues) requires the dedup window to be per (queue, group, dedupID). Two distinct MessageGroupIds that FNV-collide onto the same partition would share a dedup namespace, so a fresh send in group B reusing group A's dedup-id would be silently acked with group A's MessageId — data loss. Threaded groupID through sqsPartitionedMsgDedupKey, sqsMsgDedupKeyDispatch, and loadFifoDedupRecord.

Caller audit: loadFifoDedupRecord has exactly one production caller (sendFifoMessage at sqs_fifo.go:186); the same dedupKey returned from loadFifoDedupRecord populates both the OCC ReadKeys (line 238) and the Put write (line 243), so read-guard and write share one key. Legacy (non-partitioned) shape is unchanged on disk — the legacy branch ignores groupID, preserving compatibility for queues created before partitioning landed.

2. Fanout aliasing (sqs_messages.go:933): Round-1's XOR-fold of readTS bits aliases under structured strides — HLC packs a 16-bit logical counter in low bits and ReceiveMessage commits a fixed number of per-message transactions per call, so consecutive readTS deltas have a fixed stride. With partitions=4 and stride=10 the masked fold alternates 0,2,0,2,… and partitions 1,3 never appear as the start. Replaced with a per-server atomic.Uint32 counter — strict round-robin regardless of HLC behaviour. Counter is per-server (not Raft-replicated) because fanout distribution is a local-fairness property.

Two new regression tests:

• TestSQSDedupKeyDispatch_PartitionedScopesByMessageGroupId — distinct MessageGroupIds on the same (queue, partition, dedupID) must produce distinct dedup keys; legacy keyspace unchanged.
• TestNextReceiveFanoutStart_RoundRobin — 16 calls over 4 partitions produce exactly 4 hits per partition (strict round-robin); concurrent receivers tested under -race.

go build, go vet, golangci-lint, and the targeted SQS test suite all clean.

Claude finished @bootjp's task in 6m 25s —— View job

Code Review: Round 3 — Dedup scoping + Fanout aliasing fixes

• Gather context and read changed files
• Review dedup scoping fix (sqs_fifo.go, sqs_keys.go, sqs_keys_dispatch.go)
• Review fanout aliasing fix (sqs_messages.go, sqs.go)
• Review new regression tests
• Verify partition resolver unaffected by key format change
• Post complete review

Overall assessment

Both P1 findings from the round-2 Codex review are correctly addressed. The implementations are clean, the regression tests are well-targeted, and no new issues were introduced. Ready to merge.

Finding 1: Dedup scoping (sqs_keys.go:326, sqs_fifo.go:98, sqs_keys_dispatch.go:64) ✅

The fix correctly scopes the partitioned dedup key on (queue, partition, gen, groupID, dedupID) by threading groupID through every layer:

• sqsPartitionedMsgDedupKey: adds encodeSQSSegment(groupID) before encodeSQSSegment(dedupID)
• sqsMsgDedupKeyDispatch: passes groupID through to the partitioned branch, ignores it on the legacy branch
• loadFifoDedupRecord: new groupID parameter, one production caller (sendFifoMessage:186 passing in.MessageGroupId)
• OCC invariant preserved: the same dedupKey returned by loadFifoDedupRecord populates both ReadKeys (the read-guard) and the Put write — they stay in sync

Partition resolver unaffected: parsePartitionedSQSKey only reads the key through the partition field (prefix → queue segment → | terminator → 4 bytes for partition) and stops. The groupID and dedupID appended after gen are never parsed by the resolver. ✓

Legacy keyspace unchanged: The meta == nil || PartitionCount <= 1 branch routes to sqsMsgDedupKey, which has always been (queue, gen, dedupID) — groupID is not threaded there. On-disk compatibility preserved for pre-partitioning queues. ✓

Caller audit: sqsPartitionedMsgDedupKey has exactly two callers — sqsMsgDedupKeyDispatch (production dispatch layer) and the partition resolver test (updated). loadFifoDedupRecord has exactly one production caller (sendFifoMessage). ✓

Finding 2: Fanout aliasing (sqs.go:179, sqs_messages.go:931) ✅

The round-1 startPartitionOffset (XOR-fold of readTS bits) is fully removed and replaced with nextReceiveFanoutStart (per-server atomic.Uint32 counter).

func (s *SQSServer) nextReceiveFanoutStart(partitions uint32) uint32 {
    if partitions <= 1 {
        return 0
    }
    return s.receiveFanoutCounter.Add(1) & (partitions - 1)
}

Correctness:

• atomic.Uint32.Add is thread-safe — no torn reads under concurrent ReceiveMessage calls ✓
• Power-of-two invariant (htfifoValidatePartitionCount) makes & (partitions-1) equivalent to modulo ✓
• Counter wraps at math.MaxUint32 cleanly — the mask-AND cycle continues uninterrupted ✓
• partitions <= 1 short-circuits without calling Add, so legacy receives don't perturb the counter ✓
• receiveFanoutCounter is zero-initialized by default in NewSQSServer (via &SQSServer{...} with named fields; atomic.Uint32 zero value is 0) ✓
• startPartitionOffset fully removed — no dangling references ✓

Starting offset: The first partitioned call yields 1 & (partitions-1) (counter starts at 0, first Add(1) returns 1). This means partition 0 is reached on the 4th call for a 4-partition queue, not the 1st. The test correctly accounts for this (16 calls starting from 1 → counts: p1=4, p2=4, p3=4, p0=4 ✓).

Regression tests ✅

TestSQSDedupKeyDispatch_PartitionedScopesByMessageGroupId (sqs_keys_dispatch_test.go:171):

• Same (queue, partition, dedupID) with different groupID → distinct keys (the actual regression scenario)
• Same (group, dedupID) → identical key (idempotency contract)
• Legacy branch ignores groupID (backward compat)

TestNextReceiveFanoutStart_RoundRobin (sqs_partitioned_dispatch_test.go:355):

• partitions <= 1 → returns 0, counter unchanged
• 16 calls over 4 partitions → exactly 4 hits per partition (strict round-robin)
• 1024 calls → all offsets in [0, partitions)
• 8 goroutines × 256 calls → all concurrent results in [0, partitions) (atomicity check under -race)

These directly pin the failure modes Codex identified and are stronger than a "distribution covers all partitions" check.

Minor observations (non-blocking)

1. Theoretical base64 concatenation ambiguity (low, pre-existing)

encodeSQSSegment(groupID) + encodeSQSSegment(dedupID) without a delimiter is theoretically ambiguous: pairs with lengths |g|=2, |d|=1 and |g|=1, |d|=2 could produce the same concatenated string if specific bit patterns align at the boundary. In practice this is not exploitable with valid AWS strings (MessageGroupId and MessageDeduplicationId are constrained to alphanumeric + _-, all ASCII 0x2D–0x7A), because constructing a collision requires the padding bits at the base64 boundary to be simultaneously zero for both interpretations — a constraint the valid ASCII range essentially never satisfies. This is also a pre-existing pattern in the codebase (e.g., the group key constructor uses encodeSQSSegment(groupID) as the final segment, and the legacy dedup key uses encodeSQSSegment(dedupID) as the final segment without issue). Not a new risk and not a blocker.

2. Comment length (sqs_messages.go:907–935)

The nextReceiveFanoutStart doc block is ~24 lines. CLAUDE.md convention is "one short line max" for comments. The historical context (round-1 fix, Codex P1) is valuable for future maintainers but most of it belongs in the PR description rather than the source file. Could be trimmed to ~5 lines: the invariant, the power-of-two property, and the counter-not-replicated justification.

Correctness table (Round 3)

Verdict: Ready to merge.

@claude review

Round 4 — addressed Codex 🟠 P1 round 4: per-queue fanout counter.

The round-3 server-wide counter aliased across queues — when queues interleave with strides that share a factor with PartitionCount, a queue cycles through only a subset of partitions. Concrete: 4-partition queue receiving every other tick observes 1,3,5,7,… mod 4 = 1,3,1,3 → partitions 0 and 2 never appear.

Replaced atomic.Uint32 field with sync.Map keyed by queue name; each queue gets its own *atomic.Uint32. Each queue's rotation now depends only on its own receive cadence — strict round-robin regardless of cross-queue interleaving.

Caller audit: nextReceiveFanoutStart has one production caller (scanAndDeliverOnce at sqs_messages.go:885), which now passes queueName from existing scope. Return type and range unchanged ([0, partitions)).

Test: TestNextReceiveFanoutStart_PerQueueRoundRobin (renamed) now asserts queueB and queueC interleaved each see strict round-robin — the round-3 server-wide counter would fail this. Concurrent receivers across many queue names exercise LoadOrStore under -race.

go build, go vet, golangci-lint, and the broader SQS test suite all clean (10.5s).

Claude finished @bootjp's task in 2m 5s —— View job

Code Review: Round 4 — Per-queue fanout counter

• Gather context and read changed files
• Review per-queue counter implementation (sqs.go, sqs_messages.go)
• Verify sync.Map + atomic.Uint32 correctness and race safety
• Review renamed/updated test
• Check for any remaining issues
• Post complete review

Overall assessment

The fix is correct and the right abstraction for this problem. The sync.Map + *atomic.Uint32 pattern is the standard Go idiom for per-key counters with read-mostly access patterns. The test directly pins the exact failure mode the Codex P1 finding identified. No new issues introduced. Ready to merge.

Correctness: nextReceiveFanoutStart (sqs_messages.go:920–930)

func (s *SQSServer) nextReceiveFanoutStart(queueName string, partitions uint32) uint32 {
    if partitions <= 1 {
        return 0
    }
    v, ok := s.receiveFanoutCounters.Load(queueName)
    if !ok {
        v, _ = s.receiveFanoutCounters.LoadOrStore(queueName, &atomic.Uint32{})
    }
    counter, _ := v.(*atomic.Uint32)
    return counter.Add(1) & (partitions - 1)
}

Load → LoadOrStore idiom — correct. Load first avoids the allocation cost of &atomic.Uint32{} on every call in the common path (queue already in map). On a race between two goroutines making the queue's first receive, LoadOrStore is atomic — both goroutines get the winner's pointer from the map; the loser's allocation is immediately GC'd. The returned v is always a valid *atomic.Uint32. ✅

Type assertion — v.(*atomic.Uint32) is safe because the only value ever stored under any key is &atomic.Uint32{}. The blank discard on the ok bool is idiomatic when the type is guaranteed by construction. ✅

Non-partitioned short-circuit — partitions <= 1 returns 0 without touching the map, so unconfigured / legacy queues pay one branch check and zero map operations per call. ✅

Round-robin contract — each queue has its own counter, incremented independently. After partitions calls on the same queue, every starting offset in [0, partitions) appears exactly once. With power-of-two partitions (enforced by htfifoValidatePartitionCount), the mask-AND is equivalent to modulo. Counter wraps cleanly at math.MaxUint32 — the mask-AND cycle continues uninterrupted. ✅

The Round 4 starvation scenario is closed. The concrete example from the Codex finding: queueB receiving every other tick with a server-wide counter would observe 1,3,5,7,… mod 4 = 1,3,1,3 → partitions 0 and 2 starved. With per-queue counters, queueB's counter increments independently (1,2,3,4,5,… mod 4 = 1,2,3,0,…) → all four partitions appear as starting points in strict round-robin. ✅

Test quality: TestNextReceiveFanoutStart_PerQueueRoundRobin

The key addition is the cross-queue isolation assertion (sqs_partitioned_dispatch_test.go:399–419):

bSeen := make(map[uint32]int, partitions)
cSeen := make(map[uint32]int, partitions)
for range 16 {
    bSeen[s.nextReceiveFanoutStart("queueB", partitions)]++
    cSeen[s.nextReceiveFanoutStart("queueC", partitions)]++
}
for p := uint32(0); p < partitions; p++ {
    require.Equal(t, 4, bSeen[p], ...)
    require.Equal(t, 4, cSeen[p], ...)
}

This test would fail deterministically on the round-3 server-wide counter: B's values are every-other-tick (1,3,5,7,…) → 1,3,5,7 mod 4 = 1,3,1,3 → bSeen[0] and bSeen[2] would be 0, not 4. ✅

The concurrent sub-test now uses fmt.Sprintf("concurrent-%d", idx%3) so 8 goroutines share 3 queue names — specifically exercises the LoadOrStore path under -race when multiple goroutines race on the same queue's initial counter install. ✅

Minor observations (non-blocking)

1. Counter persists after DeleteQueue

When a queue is deleted and then recreated with the same name, the new queue reuses the previous counter from the map — its first receive starts at an arbitrary offset, not necessarily partition 0. This is correct behavior (round-robin from any starting point is still round-robin), but it's worth noting in a follow-up doc or comment if DeleteQueue behavior is ever tested for this path. Not a bug.

2. One allocation per first receive per queue per process lifetime

&atomic.Uint32{} is allocated once per distinct queue that ever receives a partitioned message. In a contention race on the initial insert, the losing allocation is GC'd immediately. The commit message correctly characterizes this as bounded by the CreateQueue rate — negligible in practice. ✅

3. Comment block length

Both the receiveFanoutCounters field doc (sqs.go:171–186) and the nextReceiveFanoutStart function comment (sqs_messages.go:908–919) exceed CLAUDE.md's "one short line max" convention by a wide margin. The historical context (round 3 → round 4 aliasing) is genuinely non-obvious and helpful, but most of it belongs in the commit message and PR description rather than the source file. Minor style nit — the information is correct, just verbose.

Correctness table (Round 4)

Verdict: Ready to merge.

@claude review

Round 5 — addressed Codex P2 (user-elevated): per-queue fanout counter eviction.

The round-4 per-queue receiveFanoutCounters sync.Map populates one entry per queue name on the first partitioned receive but never removes it. Repeated DeleteQueue+CreateQueue of unique names then leaks one entry per name for the process lifetime — a real concern in multi-tenant / high-churn deployments.

Fix: new dropReceiveFanoutCounter(queueName) helper, mirrored to the same two call sites as the existing throttle.invalidateQueue:

• createQueue genuine-create branch — covers a delete-then-create race where a partitioned receive holding pre-delete meta could repopulate the entry between the delete-side cleanup and this create commit.
• deleteQueue HTTP handler — drops the counter alongside the throttle bucket so DeleteQueue+CreateQueue resets every piece of in-memory queue state.

setQueueAttributes is intentionally NOT mirrored — it only mutates throttle config, never PartitionCount, so the counter stays valid across attribute changes (same conditional shape the throttle invalidate already uses there).

Caller audit: new helper has exactly two production callers, both adjacent to existing throttle.invalidateQueue calls. AdminDeleteQueue does not call throttle.invalidateQueue today either — staying consistent (not fixing that pre-existing inconsistency in this round).

Test: TestDropReceiveFanoutCounter_ClearsEntry pins

• no-op on never-touched queues (no allocation, no panic)
• removal after a populated counter
• per-queue isolation (dropping queueB leaves queueC intact)
• fresh allocation after drop (first offset == 1 on 4-partition queue, proving the old pointer was not recycled)

Build, golangci-lint, and the targeted SQS test sweep (TestDropReceiveFanoutCounter | TestNextReceiveFanoutStart | TestSQSServer_PartitionedFIFO | TestValidateReceiptHandleVersion, plus a wider CreateQueue|DeleteQueue|PurgeQueue|SetQueueAttributes|SendMessage|ReceiveMessage|DeleteMessage regression sweep) all clean under -race.

Claude finished @bootjp's task in 1m 53s —— View job

Code Review: Round 5 — Per-queue fanout counter eviction

• Gather PR context and understand Round 5 changes
• Read adapter/sqs_messages.go — dropReceiveFanoutCounter helper
• Read adapter/sqs_catalog.go — createQueue and deleteQueue call sites
• Read adapter/sqs_admin.go — AdminDeleteQueue gap
• Review TestDropReceiveFanoutCounter_ClearsEntry
• Verify caller audit completeness
• Check race safety and post-drop re-population path
• Post complete review

Overall assessment

The fix is correct, minimal, and faithful to the pattern it mirrors. dropReceiveFanoutCounter is a one-liner wrapping sync.Map.Delete, placed at the same two call sites as throttle.invalidateQueue. The leak scenario described in the Codex P2 finding is closed. No new issues introduced. Ready to merge.

Implementation

dropReceiveFanoutCounter (sqs_messages.go:940–942):

func (s *SQSServer) dropReceiveFanoutCounter(queueName string) {
    s.receiveFanoutCounters.Delete(queueName)
}

sync.Map.Delete is a documented no-op when the key is absent, so calling this on a queue that never issued a partitioned receive is safe. The function correctly does not check for existence first — that would introduce a TOCTOU window. ✅

createQueue call site (sqs_catalog.go:1020): placed after deleteQueueWithRetry commits and immediately after throttle.invalidateQueue. The race the comment describes is real: a partitioned ReceiveMessage holding stale pre-delete meta could call nextReceiveFanoutStart between the delete-side dropReceiveFanoutCounter and this create commit, repopulating the map. Dropping here on the genuine-create path removes that survivor. ✅

deleteQueue HTTP handler (sqs_catalog.go:1052): placed after deleteQueueWithRetry succeeds, immediately after throttle.invalidateQueue. Pattern-matches the throttle cleanup. ✅

Race analysis: dropReceiveFanoutCounter (Delete) racing with nextReceiveFanoutStart (LoadOrStore) during the delete window is safe — sync.Map operations are individually atomic. The worst outcome is a concurrent receive re-inserting an entry that the drop immediately removes, which is still correct: the next receive after delete gets a fresh zero counter via LoadOrStore. ✅

Caller audit

The AdminDeleteQueue gap is pre-existing (it also doesn't clean up the throttle), acknowledged in the commit message, and intentionally deferred. The PR's stated scope is to mirror throttle.invalidateQueue's two call sites — this is satisfied. The setQueueAttributes exclusion is justified: PartitionCount is immutable post-create, so the counter for an existing queue remains valid across attribute changes. ✅

Test: TestDropReceiveFanoutCounter_ClearsEntry

Four assertions, each pins a distinct failure mode:

The first == 1 assertion is particularly tight: it would fail if Delete removed the key but a concurrent goroutine's LoadOrStore returned the old pointer (impossible with sync.Map semantics, but the test proves it). ✅

Minor observations (non-blocking)

1. PR reference in source comment (sqs_catalog.go:1051)

// — a leak in multi-tenant / high-churn deployments. Codex P2,
// PR #732 round 5.

Per CLAUDE.md: "Don't reference the current task, fix, or callers... since those belong in the PR description and rot as the codebase evolves." The "Codex P2, PR #732 round 5" attribution belongs in the commit message (where it already is, verbatim). The comment at the createQueue call site does not have this issue. Minor style nit; the deleteQueue comment reads naturally without it.

2. dropReceiveFanoutCounter doc block length

The function comment is 9 lines. CLAUDE.md convention is "one short line max" — the non-obvious WHY fits in two lines: the leak scenario + the no-op guarantee. The rest is reachable from the commit message. Not new to Round 5 (Rounds 3–4 had the same issue on nextReceiveFanoutStart) and not blocking.

Correctness table (Round 5)

Verdict: Ready to merge.

@claude review

Round 6 — addressed CodeRabbit 🟠 Major: dedup key segment-boundary ambiguity.

sqsPartitionedMsgDedupKey was appending encodeSQSSegment(groupID) and encodeSQSSegment(dedupID) back-to-back. encodeSQSSegment uses base64.RawURLEncoding (no padding, alphabet [A-Za-z0-9_-]), so the boundary is variable-length and ambiguous: distinct (group, dedup) pairs can collapse onto the same byte sequence — most blatantly when one is empty (("", "abcd") and ("abcd", "") both produce "...QUJDRA"). This is the on-disk format PR 5b-3 will rely on, so it had to be fixed before that PR.

Fix: insert sqsPartitionedQueueTerminator | between the two segments. | is outside RawURLEncoding's alphabet so neither segment can contribute one of its own — boundary unambiguous regardless of input length. Mirrors how sqsPartitionedMsgDataKey, MsgVisKey, MsgGroupKey, and MsgByAgeKey already use the same terminator at the queue/partition boundary.

Caller audit: sqsPartitionedMsgDedupKey is reached only through sqsMsgDedupKeyDispatch, which has exactly two production callers — loadFifoDedupRecord (read-side) and sendFifoMessage (write-side, using the key returned from the same loadFifoDedupRecord call). Read and write share one helper invocation, so the new format takes effect symmetrically — no read/write skew window. parsePartitionedSQSKey only inspects the key through the partition field and stops; the new terminator after groupID does not affect parsing.

Dormancy: partitioned queues are still gated at CreateQueue (§11 PR 2, lifted atomically with the capability check in PR 5b-3), so no production data carries the old format. Pure forward-format correction.

Test (TestSqsPartitionedMsgDedupKey_GroupDedupSeparator):

• Empty-segment collision class (the most blatant ambiguity)
• New terminator exact placement (key ends with b64(group)|b64(dedup))
• Non-empty pair non-collision (("ab", "cd") vs ("abcd", ""))
• Read-write symmetry through the dispatch helper

Build, golangci-lint, and the targeted dedup/dispatch/partitioned-FIFO test sweep all clean under -race. CodeRabbit's other finding on the test (🟡 Minor on the post-delete check comment) is below the loop threshold and not addressed in this round.

Claude finished @bootjp's task in 2m 30s —— View job

Code Review: Round 6 — Dedup key segment-boundary fix

• Gather PR context and understand Round 6 changes
• Read adapter/sqs_keys.go — sqsPartitionedMsgDedupKey fix
• Read adapter/sqs_keys_dispatch_test.go — TestSqsPartitionedMsgDedupKey_GroupDedupSeparator
• Read adapter/sqs_keys_dispatch.go — sqsMsgDedupKeyDispatch caller
• Verify segment terminator placement and ambiguity closure
• Verify parsePartitionedSQSKey is unaffected
• Review test coverage for the new boundary cases
• Post complete review

Overall assessment

The Round 6 fix is correct, minimal, and the right shape. The sqsPartitionedQueueTerminator insertion is the natural fix for the ambiguity — it mirrors how every other partitioned key constructor already uses | to delimit variable-length segments. The tests are well-targeted and directly pin the failure mode. One nit remains from prior rounds (not introduced by Round 6). The deferred 🟡 Minor is acknowledged and below threshold. Ready to merge.

Correctness: sqsPartitionedMsgDedupKey (adapter/sqs_keys.go:343)

buf = append(buf, encodeSQSSegment(groupID)...)
buf = append(buf, sqsPartitionedQueueTerminator)  // ← Round 6 addition
buf = append(buf, encodeSQSSegment(dedupID)...)

Ambiguity closure — encodeSQSSegment uses base64.RawURLEncoding (no padding, no =). The RawURLEncoding alphabet is A-Za-z0-9-_; | (0x7C) is not in it. Neither segment can contribute a | byte of its own, so the terminator uniquely marks the segment boundary regardless of input length. The ("", "abcd") / ("abcd", "") collision class, and the length-mod-3 boundary cases for non-empty inputs, are all closed. ✅

Mirrors existing conventions — sqsPartitionedMsgDataKey, MsgVisKey, MsgGroupKey, MsgByAgeKey all use the same sqsPartitionedQueueTerminator after the queue name. This is now the uniform pattern. ✅

Read-write symmetry — loadFifoDedupRecord (sqs_fifo.go:99) calls sqsMsgDedupKeyDispatch and returns (rec, key, err). sendFifoMessage (sqs_fifo.go:186) gets back that same dedupKey and uses it for both ReadKeys (OCC read-guard) and the Put write (sqs_fifo.go:242,247). Both sides use the same single helper invocation — no skew window. ✅

parsePartitionedSQSKey unaffected — The resolver parses prefix → queue segment → | (terminator) → 4 bytes for partition. It stops there. The gen, groupID, |, and dedupID appended after the partition are never parsed by the resolver. The Round 6 terminator is beyond its stopping point. ✅

Dormancy safety — CreateQueue still rejects PartitionCount > 1 via validatePartitionDormancyGate. No production data was ever written in the old (ambiguous) format. The key format change is purely forward-only. ✅

Test quality: TestSqsPartitionedMsgDedupKey_GroupDedupSeparator

Four targeted assertions, each pinning a distinct failure mode:

The existing TestSQSDedupKeyDispatch_PartitionedScopesByMessageGroupId (from Round 3) remains in place and still passes, providing orthogonal coverage: it asserts that two distinct groupID values on the same (queue, partition, dedupID) produce different keys. ✅

Minor observations (not blocking)

1. Comment block length and PR reference (sqs_keys.go:334–342)

The 9-line inline comment includes "CodeRabbit major, PR #732 round 6". Per CLAUDE.md: "one short line max" and "Don't reference the current task, fix, or callers... since those belong in the PR description and rot as the codebase evolves." Same pre-existing pattern as rounds 3–5; not introduced by Round 6 and not blocking.

2. Deferred 🟡 Minor: misleading post-delete comment (sqs_partitioned_dispatch_test.go:200–202)

Explicitly deferred by the trigger comment ("below the loop threshold"). The claim that sleeping 1100ms will re-expose drain-phase invisible messages is incorrect (drain uses VTO=60), but the direct keyspace scan at lines 183-198 is the definitive correctness guard — the comment overstates the API receive's coverage. Not blocking; acknowledged.

Correctness table (Round 6)

Verdict: Ready to merge.